Skip to content

fix(workflow): prevent concurrent map writes on shared error singletons#2714

Open
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-workflow-concurrent-map-write
Open

fix(workflow): prevent concurrent map writes on shared error singletons#2714
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-workflow-concurrent-map-write

Conversation

@nankingjing

Copy link
Copy Markdown

What this PR does

Fixes a fatal error: concurrent map writes crash in workflow execution (reported in #2713).

Root cause

WorkflowError values carry an error level. The level was stored inside the
underlying errorx.StatusError's Extra() map, and ChangeErrLevel mutated
that map in place:

func (w *wfErr) ChangeErrLevel(newLevel ErrorLevel) WorkflowError {
    w.StatusError.Extra()["level"] = string(newLevel) // in-place write
    return w
}

Several WorkflowError values are package-level singletons that are shared by
every goroutine:

var CancelErr = newCancel()
var NodeTimeoutErr = newNodeTimeout()
var WorkflowTimeoutErr = newWorkflowTimeout()

When a node fails, (*nodeRunner).onError assigns one of these shared
singletons (e.g. sErr = vo.NodeTimeoutErr on context.DeadlineExceeded) and
then calls sErr.ChangeErrLevel(vo.LevelWarn). Under concurrent workflow
execution — e.g. many requests whose HTTP nodes time out at the same time —
multiple goroutines write to the same shared map simultaneously, which Go
detects as a fatal, unrecoverable concurrent map writes and crashes the
whole process. This exactly matches the stack trace in #2713
(node.go:151 in ChangeErrLevel, node_runner.go:848 in onError).

The fix

  • Give wfErr its own level ErrorLevel field instead of storing the level in
    the shared Extra() map.
  • ChangeErrLevel now returns a copy of the error with the new level and
    never mutates the receiver, so the shared singletons are never modified.
  • Level() prefers the new field and falls back to the previous Extra()
    lookup, so behavior is unchanged for existing callers (event_handle.go).
  • Constructors populate the field; the Extra("level", ...) metadata is kept
    for backward compatibility.

After the fix the singletons' Extra() map is only written once at
construction and is read-only afterwards, so concurrent access is safe.

Tests

Added node_concurrency_test.go:

  • TestChangeErrLevelIsSafeForConcurrency — 100 goroutines call
    ChangeErrLevel on the shared singletons concurrently. This reproduces the
    crash on the old code and passes on the new code (also clean under -race).
  • TestChangeErrLevelDoesNotMutateSingleton — verifies ChangeErrLevel
    returns a copy and leaves the singleton's level untouched.

Fixes #2713

@nankingjing

Copy link
Copy Markdown
Author

Review: fix-workflow-concurrent-map-write (#2714)

Verdict: LGTM

This is a well-crafted fix for a real concurrency bug. The key insight is correct: package-level singletons (CancelErr, NodeTimeoutErr, WorkflowTimeoutErr) are shared across goroutines, and the old ChangeErrLevel mutated their shared StatusError.Extra() map in place, triggering "fatal error: concurrent map writes".

What works well:

  • The new level field on wfErr is private and owned by each instance, avoiding shared mutable state entirely.
  • ChangeErrLevel now returns a copy instead of mutating the receiver -- idiomatic Go value-semantics for immutable updates.
  • All constructor functions explicitly initialize level, which is important because the zero value is handled correctly by Level().
  • The concurrency test with 100 goroutines exercising all three singletons is a solid regression guard.
  • The singleton-immutability test confirms ChangeErrLevel does not mutate the original.

No issues found. Merge-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

工作流错误处理时引发 fatal error: concurrent map writes

1 participant